Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Clarity get/set! when canget/set returns false #210

Closed
wants to merge 3 commits into from
Closed

Conversation

odow
Copy link
Member

@odow odow commented Feb 9, 2018

As raised in #209

@odow odow mentioned this pull request Feb 9, 2018
8 tasks
@mlubin
Copy link
Member

mlubin commented Feb 9, 2018

Hmm. I don't think we should allow get to return incorrect results or say that set! values can be ignored. These should be errors.

@tkoolen
Copy link
Contributor

tkoolen commented Feb 9, 2018

I think that ideally:

  • It should always be possible to get any attribute in a 'safe' way, e.g. by having a checkedget/safeget (or maybe this should be called get) that first checks canget and then returns the result. This should probably be the default behavior.
  • There should be an unsafeget version (currently called get) for at least some properties that foregoes the canget check for efficiency reasons, (e.g when you want to iterate over a bunch of VariableIndexes) and you've already checked canget for ::Type{VariableIndex}.

This is starting to sound like the @inbounds machinery. Unfortunately, that's hard to replicate, but maybe some kind of simple @unsafe macro solution to redirect to the unsafeget version would be nice.

@mlubin
Copy link
Member

mlubin commented Feb 9, 2018

@tkoolen is there really a need for an unsafe version? If you're iterating over a bunch of VariableIndexes then you can use the ::Vector{VariableIndex} version of get instead.

@blegat
Copy link
Member

blegat commented Feb 9, 2018

What would be done in unsafeget if canget is false ? The solvers returns an incorrect value and the error is thrown in safeget which is implemented in MOI like this ?

function MOI.safeget(...)
    if MOI.canget(...)
        MOI.get(...)
    else
        error(...)
    end
end

@tkoolen
Copy link
Contributor

tkoolen commented Feb 9, 2018

@mlubin, I thought that there was a need for that given your earlier comment, JuliaOpt/MathOptInterfaceBridges.jl#87 (comment). If that's not the case, what I proposed is probably unnecessary.

@blegat, yep.

@odow
Copy link
Member Author

odow commented Feb 14, 2018

This has been updated to reflect that the call should throw an error.

### Note

It is the user's responsibility to check that `canget` returns `true` prior to calling `get`.
Calling `get` when `canget` returns `false` will throw an error.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is now a weird statement. Do we need to add this note for addconstraint! modifyconstraint!, etc (anything that as a corresponding canXXX)?
I would phrase it more of a note for implementations. These functions must throw errors if the corresponding canXXX returned false.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this just be a general note in the manual then?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, could be in the section on notes for solver wrappers.

@odow odow mentioned this pull request Feb 14, 2018
@odow
Copy link
Member Author

odow commented Feb 14, 2018

Closing in favour of #228

@odow odow closed this Feb 14, 2018
@odow odow deleted the odow-patch-1 branch May 1, 2018 21:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants